Reflect changes to server selector in opaqueness #12031
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Alex Leong <[email protected]>
Signed-off-by: Alex Leong <[email protected]>
Signed-off-by: Alex Leong <[email protected]>
zaharidichev
reviewed
Feb 2, 2024
Comment on lines
210
to
216
| PodSelector: &metav1.LabelSelector{ | ||
| // PodSelector is updated to NOT select the pod | ||
| MatchLabels: map[string]string{"app": "FOOBAR"}, | ||
| }, | ||
| ExternalWorkloadSelector: &metav1.LabelSelector{ | ||
| MatchLabels: map[string]string{"app": "external-workload-policy-test"}, | ||
| }, |
There was a problem hiding this comment.
what is the significance of having the ExternalWorkloadSelector selector set here in this test. Isn't there a one-of constraint present on the podSelector and externalWorkloadSelector fields in general
| @@ -158,6 +159,110 @@ func TestGet(t *testing.T) { | |||
| } | |||
| }) | |||
|
|
|||
| t.Run("Return endpoint opaque protocol controlled by a server", func(t *testing.T) { | |||
There was a problem hiding this comment.
can we add the same test for external workloads ?
There was a problem hiding this comment.
great question. I tried this and then spent forever racking my brain trying to figure out why it wasn't working. it turns out the server_tests all run with the endpoint_watcher in Endpoints mode, not EndpointSlices mode, and external workloads are not compatible with Endpoints.
Given that EndpointSlices mode is the default, we'll want to rewrite these tests to use it, but not in this PR.
Signed-off-by: Alex Leong <[email protected]>
Signed-off-by: Alex Leong <[email protected]>
zaharidichev
approved these changes
Feb 3, 2024
mateiidavid
approved these changes
Feb 5, 2024
| if portMatch { | ||
| if isAdd && server.Spec.ProxyProtocol == opaqueProtocol { | ||
| if pp.isAddressSelected(address, oldServer) || pp.isAddressSelected(address, newServer) { | ||
| if newServer != nil && pp.isAddressSelected(address, newServer) && newServer.Spec.ProxyProtocol == opaqueProtocol { |
There was a problem hiding this comment.
Could we maybe simplify this by saying:
if pp.isAddressSelected(address, oldServer) || pp.isAddressSelected(address, newServer) {
if pp.isAddressSelected(address, newServer) && newServer.Spec.ProxyProtocol == opaqueProtocol {
}
}iiuc isAddressSelected will return false if the server it is applied to is nil so it seems redundant to check it in this if condition too.
Merged
mateiidavid
added a commit
that referenced
this pull request
Feb 9, 2024
This release addresses some issues in the destination service that could cause it to behave unexpectedly when processing updates. * Fixed a race condition in the destination service that could cause panics under very specific conditions ([#12022]; fixes [#12010]) * Changed how updates to a `Server` selector are handled in the destination service. When a `Server` that marks a port as opaque no longer selects a resource, the resource's opaqueness will reverted to default settings ([#12031]; fixes [#11995]) * Introduced Helm configuration values for liveness and readiness probe timeouts and delays ([#11458]; fixes [#11453]) (thanks @jan-kantert!) [#12010]: #12010 [#12022]: #12022 [#11995]: #11995 [#12031]: #12031 [#11453]: #11453 [#11458]: #11458 Signed-off-by: Matei David <[email protected]>
adleong
added a commit
that referenced
this pull request
Feb 17, 2024
Fixes #11995 If a Server is marking a Pod's port as opaque and then the Server's podSelector is updated to no longer select that Pod, then the Pod's port should no longer be marked as opaque. However, this update does not result in any updates from the destination API's Get stream and the port remains marked as opaque. We fix this by updating the endpoint watcher's handling of Server updates to consider both the old and the new Server. Signed-off-by: Alex Leong <[email protected]>
Merged
adleong
added a commit
that referenced
this pull request
Feb 20, 2024
This stable release back-ports bugfixes and improvements from recent edge releases. * Introduced support for arbitrary labels in the `podMonitors` field in the control plane Helm chart (thanks @jseiser!) ([#11222]; fixes [#11175]) * Added a `prometheusUrl` field for the heartbeat job in the control plane Helm chart (thanks @david972!) ([#11343]; fixes [#11342]) * Updated the Destination controller to return `INVALID_ARGUMENT` status codes properly when a `ServiceProfile` is requested for a service that does not exist. ([#11980]) * Reduced the load on the Destination controller by only processing Server updates on workloads affected by the Server ([#12017]) * Changed how updates to a `Server` selector are handled in the destination service. When a `Server` that marks a port as opaque no longer selects a resource, the resource's opaqueness will reverted to default settings ([#12031]; fixes [#11995]) * Fixed a race condition in the destination service that could cause panics under very specific conditions ([#12022]; fixes [#12010]) * Fixed an issue where inbound policy could be incorrect after certain policy resources are deleted ([#12088]) [#11222]: #11222 [#11175]: #11175 [#11343]: #11343 [#11342]: #11342 [#11980]: #11980 [#12017]: #12017 [#11995]: #11995 [#12031]: #12031 [#12010]: #12010 [#12022]: #12022 [#12088]: #12088 Signed-off-by: Alex Leong <[email protected]> Signed-off-by: David ALEXANDRE <[email protected]> Signed-off-by: Justin S <[email protected]> Co-authored-by: Oliver Gould <[email protected]> Co-authored-by: Alejandro Pedraza <[email protected]> Co-authored-by: David ALEXANDRE <[email protected]> Co-authored-by: Justin Seiser <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #11995
If a Server is marking a Pod's port as opaque and then the Server's podSelector is updated to no longer select that Pod, then the Pod's port should no longer be marked as opaque. However, this update does not result in any updates from the destination API's Get stream and the port remains marked as opaque.
We fix this by updating the endpoint watcher's handling of Server updates to consider both the old and the new Server.